Conversation
WalkthroughAdds a new multi-stage Docker build stage named Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Around line 6-7: MAX_PASTE_LENGTH is defined but not used; update the sed
invocation that sets CONFIG_FEATURE_EDITING_MAX_LEN (the sed command currently
hardcoding 8192) to use the environment variable instead by expanding
MAX_PASTE_LENGTH (use double quotes so the shell expands it, e.g., reference
"$MAX_PASTE_LENGTH" in the sed replacement), or remove the unused
MAX_PASTE_LENGTH ENV if you prefer the constant; make sure the Dockerfile's sed
line and any related uses consistently reference the same variable/symbol.
🧹 Nitpick comments (2)
Dockerfile (1)
11-13: Consider adding checksum verification for the downloaded tarball.Downloading and extracting the tarball without verifying its integrity poses a supply chain risk. BusyBox provides SHA256 checksums on their download page.
♻️ Proposed fix to add checksum verification
-RUN wget https://busybox.net/downloads/busybox-${BUSYBOX_VERSION}.tar.bz2 && \ +RUN wget https://busybox.net/downloads/busybox-${BUSYBOX_VERSION}.tar.bz2 && \ + wget https://busybox.net/downloads/busybox-${BUSYBOX_VERSION}.tar.bz2.sha256 && \ + sha256sum -c busybox-${BUSYBOX_VERSION}.tar.bz2.sha256 && \ tar xf busybox-${BUSYBOX_VERSION}.tar.bz2 && \ mv busybox-${BUSYBOX_VERSION} busyboxtests.yaml (1)
4-7: LGTM on the smoke test. Consider adding a test for the extended paste buffer.The test validates that the
busyboxbinary is present and functional. However, since the PR's main purpose is to fix the max paste limit, consider adding a test that verifies the extendedCONFIG_FEATURE_EDITING_MAX_LENsetting is effective (e.g., testing input larger than 2k characters), if the test framework supports it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Dockerfiletests.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: build
🔇 Additional comments (1)
Dockerfile (1)
3-4: Verify: Does the customized BusyBox need to replace/bin/sh?The comment on line 3 states "Update max buffer length for /bin/sh", but the binary is copied to
/bin/busybox(line 234), not/bin/sh. If the goal is to fix the paste limit in the shell, you may need to either:
- Replace
/bin/shwith this custom BusyBox build, or- Create a symlink from
/bin/shto/bin/busyboxIf the intent is only to provide a custom
busyboxcommand (not fix/bin/sh), consider updating the comment to reflect the actual behavior.Also applies to: 234-234
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Dockerfile
Outdated
| ENV BUSYBOX_VERSION="1.37.0" \ | ||
| MAX_PASTE_LENGTH="65536" |
There was a problem hiding this comment.
MAX_PASTE_LENGTH is defined but not used; hardcoded value appears inconsistent.
MAX_PASTE_LENGTH is set to 65536 but the sed command on line 18 hardcodes 8192 for CONFIG_FEATURE_EDITING_MAX_LEN. This seems like a bug—either use the environment variable or remove it to avoid confusion.
🐛 Proposed fix to use the environment variable
RUN make defconfig && \
- sed -i 's/CONFIG_FEATURE_EDITING_MAX_LEN=.*/CONFIG_FEATURE_EDITING_MAX_LEN=8192/' .config && \
+ sed -i "s/CONFIG_FEATURE_EDITING_MAX_LEN=.*/CONFIG_FEATURE_EDITING_MAX_LEN=${MAX_PASTE_LENGTH}/" .config && \
sed -i 's/CONFIG_TC=y/CONFIG_TC=n/' .config && \
yes "" | make oldconfigNote: Use double quotes to enable variable expansion.
Also applies to: 17-20
🤖 Prompt for AI Agents
In `@Dockerfile` around lines 6 - 7, MAX_PASTE_LENGTH is defined but not used;
update the sed invocation that sets CONFIG_FEATURE_EDITING_MAX_LEN (the sed
command currently hardcoding 8192) to use the environment variable instead by
expanding MAX_PASTE_LENGTH (use double quotes so the shell expands it, e.g.,
reference "$MAX_PASTE_LENGTH" in the sed replacement), or remove the unused
MAX_PASTE_LENGTH ENV if you prefer the constant; make sure the Dockerfile's sed
line and any related uses consistently reference the same variable/symbol.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@Dockerfile`:
- Line 233: The Dockerfile copies a custom BusyBox binary with COPY
--from=busybox /busybox/busybox /bin/busybox but does not update the default
shell link, so /bin/sh may still point to the old ash; after that COPY, create
or update the symlink so /bin/sh points to /bin/busybox by running a command to
force-create the symlink (e.g., use ln -sf /bin/busybox /bin/sh) to ensure the
custom BusyBox is used for shell operations.
♻️ Duplicate comments (1)
Dockerfile (1)
6-7:MAX_PASTE_LENGTHis defined but not used in thesedcommand.The environment variable
MAX_PASTE_LENGTHis set to65536, but thesedcommand on line 18 hardcodes8192. This inconsistency was flagged in a previous review.🐛 Proposed fix to use the environment variable
RUN make defconfig && \ - sed -i 's/CONFIG_FEATURE_EDITING_MAX_LEN=.*/CONFIG_FEATURE_EDITING_MAX_LEN=8192/' .config && \ + sed -i "s/CONFIG_FEATURE_EDITING_MAX_LEN=.*/CONFIG_FEATURE_EDITING_MAX_LEN=${MAX_PASTE_LENGTH}/" .config && \ yes "" | make oldconfigNote: Double quotes are required for shell variable expansion.
Also applies to: 17-19
🧹 Nitpick comments (1)
Dockerfile (1)
11-13: Consider adding checksum verification for the downloaded tarball.The BusyBox source is downloaded over HTTPS without integrity verification. Adding a SHA256 checksum check would protect against tampered downloads or CDN compromises.
🔒 Proposed fix to add checksum verification
+ENV BUSYBOX_SHA256="b8cc24c9574d809e7279c3be349795c5d5ceb6fdf19ca709f80cde50e47de314" + RUN wget https://busybox.net/downloads/busybox-${BUSYBOX_VERSION}.tar.bz2 && \ + echo "${BUSYBOX_SHA256} busybox-${BUSYBOX_VERSION}.tar.bz2" | sha256sum -c - && \ tar xf busybox-${BUSYBOX_VERSION}.tar.bz2 && \ mv busybox-${BUSYBOX_VERSION} busyboxNote: Verify the correct SHA256 hash from the official BusyBox release page.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Dockerfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build
- GitHub Check: build
🔇 Additional comments (1)
Dockerfile (1)
3-4: LGTM on the overall build stage structure.The multi-stage build approach for compiling a custom BusyBox is well-structured:
- Isolated build stage avoids bloating the final image
- Build dependencies are correctly installed
WORKDIRis properly set before compilationAlso applies to: 9-9, 15-15, 21-21
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
|
||
| WORKDIR /usr/src/code | ||
|
|
||
| COPY --from=busybox /busybox/busybox /bin/busybox |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what /bin/sh points to in the Alpine base image
docker run --rm php:8.4.11-cli-alpine3.22 ls -la /bin/sh /bin/busybox 2>/dev/null || echo "Could not inspect symlinks"
docker run --rm php:8.4.11-cli-alpine3.22 readlink -f /bin/shRepository: appwrite/docker-base
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Explore the Dockerfile context around line 233
head -n 240 Dockerfile | tail -n 20Repository: appwrite/docker-base
Length of output: 1325
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about busybox/sh in the codebase
rg -i "busybox|/bin/sh" --type-list | head -20
rg -i "busybox" . -A 3 -B 3Repository: appwrite/docker-base
Length of output: 2209
🏁 Script executed:
#!/bin/bash
# Check entire Dockerfile for any sh symlink setup
grep -n "sh\|symlink\|ln -s" DockerfileRepository: appwrite/docker-base
Length of output: 229
🌐 Web query:
Alpine Linux busybox /bin/sh symlink default
💡 Result:
By default Alpine links /bin/sh to BusyBox's ash (the BusyBox-provided /bin/ash implementation). You can replace it (e.g. install dash-binsh or bash) if you need a different /bin/sh. [1][2]
Create a symlink from /bin/sh to /bin/busybox after copying the binary.
Alpine defaults to linking /bin/sh to BusyBox's ash. The Dockerfile copies a custom busybox (with CONFIG_FEATURE_EDITING_MAX_LEN=8192) to /bin/busybox, but this won't update /bin/sh unless you explicitly establish the symlink. Add RUN ln -sf /bin/busybox /bin/sh after the COPY statement to ensure the custom binary is actually used for shell operations.
🤖 Prompt for AI Agents
In `@Dockerfile` at line 233, The Dockerfile copies a custom BusyBox binary with
COPY --from=busybox /busybox/busybox /bin/busybox but does not update the
default shell link, so /bin/sh may still point to the old ash; after that COPY,
create or update the symlink so /bin/sh points to /bin/busybox by running a
command to force-create the symlink (e.g., use ln -sf /bin/busybox /bin/sh) to
ensure the custom BusyBox is used for shell operations.
1 participant
What does this PR do?
Fixes max paste limit
Test Plan
New tests
Related PRs and Issues
x
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.